feat: add support for new setAllowHardBoundTokens field. #3467

rmehta19 · 2024-12-14T05:22:06Z

Introduce new setAllowHardBoundTokens field.

rmehta19 · 2025-01-07T19:15:22Z

@lqiu96 , @blakeli0 , @zhumin8, please review, thanks!

rmehta19 · 2025-01-08T00:11:40Z

cc: @rockspore

rockspore

Thanks for the PR, Riya! Not up to me but LGTM.

rockspore · 2025-01-08T00:16:35Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

@@ -126,6 +127,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
  @Nullable private final Boolean allowNonDefaultServiceAccount;
  @VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig;
  @Nullable private final MtlsProvider mtlsProvider;
+  @Nullable private final ArrayList<String> allowedValues;


Should this variable name be more specific? Reading it alone doesn't tell you where these values will be used.

Thanks for calling this out! I updated this variable name to allowedHardBoundTokenTypes, WDYT?

LGTM. Thanks.

rmehta19 · 2025-01-15T17:29:57Z

Friendly ping @lqiu96 , @blakeli0 , @zhumin8. Thanks!

blakeli0 · 2025-01-15T18:06:20Z

What is the usage of this new allowHardBoundTokens field? Do you mind providing more background info?

blakeli0 · 2025-01-15T21:47:08Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/TransportChannelProvider.java

@@ -97,6 +98,9 @@ public interface TransportChannelProvider {
   */
  TransportChannelProvider withEndpoint(String endpoint);

+  /** Sets the allowed hard bound token types. */
+  TransportChannelProvider setAllowHardBoundTokens(ArrayList<String> allowedValues);


I think this field is going to be only used by gRPC transport per the internal doc? And the teams that would use it are all handwritten libraries? If that's the case, I think they are likely going to initialize an InstantiatingGrpcChannelProvider in their repo directly and pass it to Gax, so can we only add this to InstantiatingGrpcChannelProvider?

I think this field is going to be only used by gRPC transport per the internal doc?

That is correct, hard bound tokens is only for gRPC.

I think they are likely going to initialize an InstantiatingGrpcChannelProvider in their repo directly and pass it to Gax, so can we only add this to InstantiatingGrpcChannelProvider?

I think this will work since I believe the libraries separate their logic for HTTP and gRPC, so they don't end up using the interface TransportChannelProvider. Looking at GCS for example, this is the case

@rockspore can you confirm that the places you want to use this setting are using the InstantiatingGrpcChannelProvider directly, not the interface TransportChannelProvider?

For Cloud Spanner, I found this so it seems to be the case too, although I don't see where it gets called by default.

So yeah it should be good if we only do this in InstantiatingGrpcChannelProvider, as long as we have a way to set these default values eventually to all auto-generated clients.

@rockspore The link you referred to is a generated default instance of InstantiatingGrpcChannelProvider, which I believe will be overridden later in their handwritten code.
Either way, they are using InstantiatingGrpcChannelProvider directly not the interface. And since it is used within InstantiatingGrpcChannelProvider only, we don't need to expose a getter for it either. Hence it should be fine if we only add it to InstantiatingGrpcChannelProvider.

Thanks for the details! It makes perfect sense.

Thanks all for the discussion! Done in 60dafd0

rmehta19 · 2025-01-15T23:12:27Z

What is the usage of this new allowHardBoundTokens field? Do you mind providing more background info?

I believe we discussed this in the chat offline. To summarize it'll be used by clients (libraries) to indicate if they want MTLS / Directpath bound tokens. Please let me know if there is anything we can make more clear.

…l provider.

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

zhumin8

Can you please also add some test coverage for this change?

zhumin8 · 2025-01-17T19:59:43Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+     * <p>allowedValues is {HardBoundTokenTypes.ALTS}: If DirectPath is used to create the channel,
+     * use hard ALTS-bound tokens for requests sent on that channel.
+     *
+     * <p>allowedValues is {HardBoundTokenTypes.MTLS_S2A}: If MTLS via S2A is used to create the
+     * channel, use hard MTLS-bound tokens for requests sent on that channel.
+     *
+     * <p>allowedValues is {HardBoundTokenTypes.ALTS, HardBoundTokenTypes.MTLS_S2A}: if DirectPath
+     * is used to create the channel, use hard ALTS-bound tokens for requests sent on that channel.
+     * If MTLS via S2A is used to create the channel, use hard MTLS-bound tokens for requests sent
+     * on that channel.


These logic is not part of this setter, I suppose is part of a subsequent PR? Perhaps move this explanation there. In the future, if this logic changes, it is easy to miss updating here.

Agree that maybe the detailed explanation can be moved to the subsequent PR. The public doc for this method should be more like "What is HardBoundTokenTypes?", not "What would happen if we set HardBoundTokenTypes to different values?". But if listing the different behavior is the best way to explain "What is HardBoundTokenTypes?", I think this is OK too.

Thanks for calling this out! Looking at this again agreed that the behavior of setting the field to different values should be paired with the logic. Updated the javadoc to specify what the field means. Also made the enum javadoc a bit more specific.

90a32af

zhumin8 · 2025-01-17T20:08:06Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+   * or S2A is used to estabilsh a connection to Google APIs.
+   *
+   */
+  public enum HardBoundTokenTypes {


Does it make sense to also mark this as internal?

Agree on making this internal as well.
Thinking twice about it though, I see that it is an Experimental feature, is it that we will always set the tokens to certain values? Or it's just this feature is not stable yet, internal teams could still set this to different values? If it's the former, then we don't have to introduce another public enum since they would be obsolete soon.

Agreed that this should be marked as Internal Api, since this is intended to be set by client libraries. Done in 591ef68

This is being marked as experimental for now, since we are in progress of adding the related logic (e.g #3548, #3572) and then piloting, as discussed in the internal doc + chat. When the feature is non-experimental, the field (allowedHardBoundTokenTypes) will be set for all gapics to include both (MTLS_S2A and ALTS), however handwritten libraries will continue to set this field (allowedHardBoundTokenTypes) themselves in their handwritten layer (e.g. GCS). Additionally, when it is non-experimental, gapics + handwritten libraries will have the option to override the default value of the allowedHardBoundTokenTypes. I think the enum helps to proves clarity on the options.

rmehta19 · 2025-01-21T20:46:28Z

Can you please also add some test coverage for this change?

Added to testToBuilder. Since we haven't added a InstantiatingGrpcChannelProvider.Builder.getAllowHardBoundTokenTypes(), I just added the setter to the test.

a7f8384

rockspore · 2025-01-21T22:30:58Z

Can you please also add some test coverage for this change?

Added to testToBuilder. Since we haven't added a InstantiatingGrpcChannelProvider.Builder.getAllowHardBoundTokenTypes(), I just added the setter to the test.

a7f8384

JFYI, this will be more meaningfully tested in #3572 (not ready for review yet) and later on when MTLS logic is added.

blakeli0 · 2025-01-21T23:00:09Z

Can you please also add some test coverage for this change?

Added to testToBuilder. Since we haven't added a InstantiatingGrpcChannelProvider.Builder.getAllowHardBoundTokenTypes(), I just added the setter to the test.

a7f8384

Since we don't expose a getter and we don't need one, I think this is good enough for now. I guess we are going to test in the subsequent PR that the channel would be created differently when passing different values?

rmehta19 · 2025-01-22T00:25:18Z

@blakeli0, One thing: when we are setting this field from the gapic generator, is it ok to have this enum? IIUC the gapic generator would then have to depend on gax-grpc in order to be able to set the field to {MTLS_S2A, ALTS} for example. Is this ok? If not, should we switch this back to a string?

cc: @lqiu96, @zhumin8 since I recall we discussed these changes to gapic generator in the internal doc.

rmehta19 · 2025-01-22T00:34:32Z

Can you please also add some test coverage for this change?

Added to testToBuilder. Since we haven't added a InstantiatingGrpcChannelProvider.Builder.getAllowHardBoundTokenTypes(), I just added the setter to the test.
a7f8384

Since we don't expose a getter and we don't need one, I think this is good enough for now. I guess we are going to test in the subsequent PR that the channel would be created differently when passing different values?

That's correct. @rockspore 's #3572 for example will likely introduce some tests that make sure that the correct credential (one that can fetch hard ALTS bound tokens) is created when this list includes ALTS and when DirectPath gets picked.

blakeli0 · 2025-01-22T16:16:28Z

/gcbrun

blakeli0 · 2025-01-22T16:19:21Z

@blakeli0, One thing: when we are setting this field from the gapic generator, is it ok to have this enum? IIUC the gapic generator would then have to depend on gax-grpc in order to be able to set the field to {MTLS_S2A, ALTS} for example. Is this ok? If not, should we switch this back to a string?

cc: @lqiu96, @zhumin8 since I recall we discussed these changes to gapic generator in the internal doc.

Yes it does. Using java-language as an example, the generated code would likely be added to this method.

rmehta19 · 2025-01-22T16:55:41Z

@blakeli0, One thing: when we are setting this field from the gapic generator, is it ok to have this enum? IIUC the gapic generator would then have to depend on gax-grpc in order to be able to set the field to {MTLS_S2A, ALTS} for example. Is this ok? If not, should we switch this back to a string?
cc: @lqiu96, @zhumin8 since I recall we discussed these changes to gapic generator in the internal doc.

Yes it does. Using java-language as an example, the generated code would likely be added to this method.

Thanks for the code pointer!

blakeli0

LGTM. Please change the PR title to follow the conventional commits.

zhumin8

LGTM

rmehta19 · 2025-01-22T20:31:01Z

LGTM. Please change the PR title to follow the conventional commits.

Done.

rmehta19 · 2025-01-22T20:31:48Z

@zhumin8 / @blakeli0 would you be able to merge (as I don't have write access)? Thanks!

🤖 I have created a release *beep* *boop* --- <details><summary>2.52.0</summary> ## [2.52.0](v2.51.1...v2.52.0) (2025-01-24) ### Features * add support for new setAllowHardBoundTokens field. ([#3467](#3467)) ([38431a2](38431a2)) * revert [#3400](#3400): reintroduce experimental S2A integration in client libraries grpc transport ([#3548](#3548)) ([65a0f11](65a0f11)) ### Dependencies * update dependency com.google.api-client:google-api-client-bom to v2.7.2 ([#3578](#3578)) ([f6e5ad9](f6e5ad9)) * update dependency commons-codec:commons-codec to v1.17.2 ([#3557](#3557)) ([07ce801](07ce801)) * update dependency gitpython to v3.1.44 ([#3559](#3559)) ([e924db0](e924db0)) * update dependency org.checkerframework:checker-qual to v3.48.4 ([#3560](#3560)) ([a4726e9](a4726e9)) * update dependency smmap to v5.0.2 ([#3561](#3561)) ([6cd5d0d](6cd5d0d)) * update docker.io/library/alpine docker tag to v3.21.1 ([#3551](#3551)) ([edd5a4c](edd5a4c)) * update docker.io/library/alpine docker tag to v3.21.2 ([#3580](#3580)) ([f577ecd](f577ecd)) * update docker.io/library/maven:3.9.9-eclipse-temurin-11-alpine docker digest to 9a259c6 ([#3554](#3554)) ([eb2cbd6](eb2cbd6)) * update docker.io/library/python:3.13.1-alpine3.20 docker digest to 9ab3b6e ([#3555](#3555)) ([40a74fe](40a74fe)) * update google auth library dependencies to v1.31.0 ([#3577](#3577)) ([7fa879a](7fa879a)) * update googleapis/java-cloud-bom digest to c7c443f ([#3579](#3579)) ([fcf40b7](fcf40b7)) * update repo-automation-bots digest to 0a12b5d ([#3464](#3464)) ([b9c9d21](b9c9d21)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

Add support for new setAllowHardBoundTokens field.

09eb2b2

product-auto-label bot added the size: m Pull request size is medium. label Dec 14, 2024

Merge branch 'main' into add-setting

db51f4e

rockspore reviewed Jan 8, 2025

View reviewed changes

rmehta19 added 3 commits January 9, 2025 06:42

Merge branch 'main' into add-setting

fd1015e

Clarify variable name in InstantiatingGrpcChannelProvider.

960084d

Merge branch 'main' into add-setting

ddb3b79

blakeli0 reviewed Jan 15, 2025

View reviewed changes

add setter for allowed hard bound token types to only the GRPC channe…

60dafd0

…l provider.

product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Jan 16, 2025

blakeli0 reviewed Jan 16, 2025

View reviewed changes

List not ArrayList, no setter, InternalApi.

e193cc9

product-auto-label bot added size: xs Pull request size is extra small. and removed size: s Pull request size is small. labels Jan 16, 2025

blakeli0 reviewed Jan 16, 2025

View reviewed changes

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Show resolved Hide resolved

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated Show resolved Hide resolved

add enum + javadocs.

0e40f22

product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Jan 16, 2025

rockspore mentioned this pull request Jan 17, 2025

feat: enable DirectPath bound token in InstantiatingGrpcChannelProvider #3572

Open

Merge branch 'main' into add-setting

6a27af6

zhumin8 reviewed Jan 17, 2025

View reviewed changes

rmehta19 added 3 commits January 21, 2025 11:26

mark enum as internal only.

591ef68

improve javadoc for setter.

90a32af

add to existing unit test.

a7f8384

Merge branch 'main' into add-setting

4fe44cb

blakeli0 approved these changes Jan 22, 2025

View reviewed changes

zhumin8 approved these changes Jan 22, 2025

View reviewed changes

rmehta19 changed the title ~~Add support for new setAllowHardBoundTokens field.~~ feat: add support for new setAllowHardBoundTokens field. Jan 22, 2025

blakeli0 merged commit 38431a2 into googleapis:main Jan 22, 2025
44 of 47 checks passed

release-please bot mentioned this pull request Jan 22, 2025

chore(main): release 2.52.0 #3576

Merged

blakeli0 mentioned this pull request Jan 22, 2025

feat: revert #3400: reintroduce experimental S2A integration in client libraries grpc transport #3548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for new setAllowHardBoundTokens field. #3467

feat: add support for new setAllowHardBoundTokens field. #3467

rmehta19 commented Dec 14, 2024 •

edited

Loading

rmehta19 commented Jan 7, 2025

rmehta19 commented Jan 8, 2025

rockspore left a comment

rockspore Jan 8, 2025

rmehta19 Jan 9, 2025

rockspore Jan 9, 2025

rmehta19 commented Jan 15, 2025

blakeli0 commented Jan 15, 2025

blakeli0 Jan 15, 2025

rmehta19 Jan 15, 2025

rockspore Jan 16, 2025

blakeli0 Jan 16, 2025

rockspore Jan 16, 2025

rmehta19 Jan 16, 2025

rmehta19 commented Jan 15, 2025

zhumin8 left a comment

zhumin8 Jan 17, 2025

blakeli0 Jan 17, 2025

rmehta19 Jan 21, 2025

zhumin8 Jan 17, 2025

blakeli0 Jan 17, 2025

rmehta19 Jan 21, 2025

rmehta19 commented Jan 21, 2025

rockspore commented Jan 21, 2025

blakeli0 commented Jan 21, 2025

rmehta19 commented Jan 22, 2025 •

edited

Loading

rmehta19 commented Jan 22, 2025

blakeli0 commented Jan 22, 2025

blakeli0 commented Jan 22, 2025

rmehta19 commented Jan 22, 2025

blakeli0 left a comment

zhumin8 left a comment

rmehta19 commented Jan 22, 2025

rmehta19 commented Jan 22, 2025

feat: add support for new setAllowHardBoundTokens field. #3467

feat: add support for new setAllowHardBoundTokens field. #3467

Conversation

rmehta19 commented Dec 14, 2024 • edited Loading

rmehta19 commented Jan 7, 2025

rmehta19 commented Jan 8, 2025

rockspore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Jan 15, 2025

blakeli0 commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Jan 15, 2025

zhumin8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Jan 21, 2025

rockspore commented Jan 21, 2025

blakeli0 commented Jan 21, 2025

rmehta19 commented Jan 22, 2025 • edited Loading

rmehta19 commented Jan 22, 2025

blakeli0 commented Jan 22, 2025

blakeli0 commented Jan 22, 2025

rmehta19 commented Jan 22, 2025

blakeli0 left a comment

Choose a reason for hiding this comment

zhumin8 left a comment

Choose a reason for hiding this comment

rmehta19 commented Jan 22, 2025

rmehta19 commented Jan 22, 2025

rmehta19 commented Dec 14, 2024 •

edited

Loading

rmehta19 commented Jan 22, 2025 •

edited

Loading